Skip to content

PR#1

Open
MichalisG wants to merge 17 commits intoinitialfrom
master
Open

PR#1
MichalisG wants to merge 17 commits intoinitialfrom
master

Conversation

@MichalisG
Copy link
Owner

No description provided.

Copy link

@ppoliani ppoliani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Replace all let with const
  2. Avoid using function; prefer const instead

app.js Outdated
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

async 👍

app.js Outdated
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one suggestion; since we're using the functional programming paradigm we really like to follow the principles from the style of programming.

one of the suggestion is to use name binding. This applies for functions, as well.

This can be rewritten like this:

const run = async () => {}

app.js Outdated
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer const over let. It's nice to keep reference transparency

app.js Outdated
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here const

app.js Outdated
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

await CappedMintableToken.methods.mint(TokenDistribution.options.address, 15000, {from: account.address})

app.js Outdated
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const deployContract = (contractPath, account, args) => {}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import "openzeppelin-solidity/token/ERC20/ERC20Capped.sol";

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can remove this function because it will be inherited from the ERC20Capped

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace missing

app.js Outdated
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use promisify https://nodejs.org/dist/latest-v8.x/docs/api/util.html and then use async/await

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments